-
Notifications
You must be signed in to change notification settings - Fork 582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sk-inet: Support IP_TRANSPARENT and IPV6_TRANSPARENT socket options #2357
base: criu-dev
Are you sure you want to change the base?
sk-inet: Support IP_TRANSPARENT and IPV6_TRANSPARENT socket options #2357
Conversation
Signed-off-by: Juntong Deng <juntong.deng@outlook.com>
IP_TRANSPARENT and IPV6_TRANSPARENT are used in transparent proxies. Support for these two socket options is useful for restoring transparent proxies. Signed-off-by: Juntong Deng <juntong.deng@outlook.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## criu-dev #2357 +/- ##
============================================
+ Coverage 70.21% 70.72% +0.50%
============================================
Files 132 135 +3
Lines 34372 32907 -1465
============================================
- Hits 24134 23272 -862
+ Misses 10238 9635 -603 ☔ View full report in Codecov by Sentry. |
Please reorder patches, c/r support must be first patch and a test for it must be second patch. This way you would not break tests on bisect. |
@@ -21,6 +21,7 @@ message ip_opts_entry { | |||
optional bool pktinfo = 5; | |||
optional uint32 tos = 6; | |||
optional uint32 ttl = 7; | |||
optional bool transparent = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excess tab -> bad allignment.
This PR tries to do a good thing. But actually there it misses a bigger problem with IP_TRANSPARENT option, this option allows to bind to non-local address, so if we modify the test a bit to actually do the bind to non-local address:
We see that nothing works:
Just dumping and restoring IP_TRANSPARENT is not enough, it should be properly integrated with the bind stage (I doubt that any app which uses it uses it without bind to non-local adress). Note that we have exactly the same problem with IP_FREEBIND (on ipv4) and to actually check IP_TRANSPARENT you need to:
|
I think the cause of this problem is that in the current open_inet_sk() of sk-inet.c, the socket options are restored at the end, and listen(), connect(), bind() are all called before setsockopt(). I think we should change this order and set all socket options after creating the socket, before listen(), connect(), bind(). What do you think? If you agree, I can create a new pull request. |
Also one more possible complication: 1) set IP_TRANSPARENT 2) bind to non-local ip 3) unset IP_TRANSPARENT, this way we somehow need to guess to set this before bind and then unset it to restore properly. So maybe we just need to extend 73a739b ("inet: set IP_FREEBIND before binding socket to an ipv6 address (v2)") to ipv4 to fix that. |
A friendly reminder that this PR had no activity for 30 days. |
Support for IP_TRANSPARENT and IPV6_TRANSPARENT socket options is
useful for restoring transparent proxies.